Core: Make metrics reporter serializable#7370
Conversation
nastra
force-pushed
the
serializable-metrics-reporter
branch
4 times, most recently
from
fb3fc4a to
5830956
Compare
nastra
force-pushed
the
serializable-metrics-reporter
branch
from
5830956 to
97aeb00
Compare
nastra
force-pushed
the
serializable-metrics-reporter
branch
2 times, most recently
from
7ccb9d8 to
8276986
Compare
| String impl = implFromLocation(location); | ||
| FileIO io = ioInstances.get(impl); | ||
| if (io != null) { | ||
| if (io instanceof HadoopFileIO && ((HadoopFileIO) io).conf() == null) { |
There was a problem hiding this comment.
Why does this need to change IO classes?
There was a problem hiding this comment.
Sorry, forgot to mention why I added this. It appears that the underlying conf of HadoopFileIO wasn't initialized when calling StaticTableOperations.current():
nastra
force-pushed
the
serializable-metrics-reporter
branch
4 times, most recently
from
8e34d90 to
35c58f6
Compare
| import java.util.Set; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.Sets; | ||
|
|
||
| public class SerializableSet<E> implements Set<E>, Serializable { |
There was a problem hiding this comment.
the idea here is aligned with SerializableMap. We need SerializableSet in the CompositeMetricsReporter to be able to serialize it
nastra
force-pushed
the
serializable-metrics-reporter
branch
2 times, most recently
from
15e0566 to
5c8199e
Compare
| } else { | ||
| assertThat(headers).containsAllEntriesOf(contextHeaders); | ||
| adaptor = | ||
| Mockito.spy( |
There was a problem hiding this comment.
this is only wrapping the RESTCatalogAdapter in a Mockito.spy() call
nastra
force-pushed
the
serializable-metrics-reporter
branch
from
5c8199e to
21a2402
Compare
| \ java.lang.String>, org.apache.iceberg.rest.RESTClient>===)" | ||
| new: "parameter void org.apache.iceberg.rest.RESTCatalog::<init>(===org.apache.iceberg.util.SerializableFunction<java.util.Map<java.lang.String,\ | ||
| \ java.lang.String>, org.apache.iceberg.rest.RESTClient>===)" | ||
| justification: "Switching to SerializableFunction" |
There was a problem hiding this comment.
Can we support both? I don't think we can make this change if it is a breaking one.
There was a problem hiding this comment.
I don't think we can support both because there's no place where we could just cast from Function to SerializableFunction
| this.encryption = table.encryption(); | ||
| this.locationProvider = table.locationProvider(); | ||
| this.refs = SerializableMap.copyOf(table.refs()); | ||
| this.metricsReporter = table.metricsReporter(); |
There was a problem hiding this comment.
Can we expose this through BaseTable instead of the Table interface? I'm not sure that we want this in the Table interface and it would be best to avoid it.
nastra
force-pushed
the
serializable-metrics-reporter
branch
from
21a2402 to
090604d
Compare
|
Closing this in favor of #8032 that doesn't require breaking changes. |
2 participants
No description provided.